Conversation
This updates the step title and description as per revamp mocks.
This adds a new dropdown to the package table toolbar that allows to switch between individual package and package group search.
This replaces the original input that only filtered the table of packages with a input with dropdown. The packages can be searched in the input and the results are rendered in the dropdown below the input.
This follows the logic of the checkboxes and copies it to set the behaviour for the dropdown options.
This removes the Selected/Available toggle and with it the logic for rendering the relevant states. The packages table should now render only selected items. Both individual packages and package groups should be visible next to each other.
This removes the previously used Included and Other repos tabs. The functionality was replaced by a button that renders in the dropdown when there are no results in the included repositories.
This removes the previously used checkbox and adds a button that only removes the selected blueprint. All functionality connected to adding a package was removed from the packages table.
This removes the expandable for individual packages and moves the list of packages inside a package group from the popover to the expandable.
This updates the "search in other repos" empty states as per mocks.
cb7ca6c to
591750e
Compare
| useListRepositoriesQuery({ | ||
| url: epelRepoUrlByDistribution, | ||
| origin: ContentOrigin.EXTERNAL, | ||
| origin: ContentOrigin.COMMUNITY, |
There was a problem hiding this comment.
This was a pre-existing leftover from the content updates. The external EPEL repos have been phased out and were replaced by the community ones.
src/Components/CreateImageWizard/steps/Packages/components/PackageSearch.tsx
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #4261 +/- ##
==========================================
+ Coverage 71.53% 71.63% +0.10%
==========================================
Files 200 194 -6
Lines 7538 7467 -71
Branches 2819 2780 -39
==========================================
- Hits 5392 5349 -43
+ Misses 1872 1845 -27
+ Partials 274 273 -1
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
591750e to
8043ce3
Compare
5422161 to
3a28ff9
Compare
|
Slightly dreading the sourcery review tbh... 😬 |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new PackageSearch component combines a lot of concerns (query orchestration, debouncing, selection behavior, UI rendering, and EPEL fallback) into ~800 lines; consider splitting it into smaller hooks/components (e.g., a search hook and a presentational select component) to improve readability and future maintainability.
- The logic for adding/removing packages and groups, especially the EPEL-related cleanup (removing recommended repositories when the last recommended item is removed), is now duplicated between PackageSearch and PackagesTable; centralizing this behavior in shared helpers or Redux-side utilities would reduce the risk of divergence on future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new PackageSearch component combines a lot of concerns (query orchestration, debouncing, selection behavior, UI rendering, and EPEL fallback) into ~800 lines; consider splitting it into smaller hooks/components (e.g., a search hook and a presentational select component) to improve readability and future maintainability.
- The logic for adding/removing packages and groups, especially the EPEL-related cleanup (removing recommended repositories when the last recommended item is removed), is now duplicated between PackageSearch and PackagesTable; centralizing this behavior in shared helpers or Redux-side utilities would reduce the risk of divergence on future changes.
## Individual Comments
### Comment 1
<location path="src/Components/CreateImageWizard/steps/Packages/components/PackageSearch.tsx" line_range="205-214" />
<code_context>
+ epelRepoUrlByDistribution,
+ ]);
+
+ useEffect(() => {
+ if (
+ wizardMode !== 'create' &&
+ isSuccessDistroRepositories &&
+ modules.length > 0
+ ) {
+ searchModulesInfo({
+ apiSearchModuleStreamsRequest: {
+ rpm_names: modules.map((module) => module.name),
+ urls: [...distroUrls, epelRepoUrlByDistribution],
+ uuids: [],
+ },
+ });
+ }
+ }, [isSuccessDistroRepositories, modules, distroUrls]);
+
+ useEffect(() => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Missing effect dependencies may lead to stale module search results when distribution or wizard mode changes.
This effect reads `wizardMode` and `epelRepoUrlByDistribution`, but they’re not listed as dependencies. If either changes, the effect won’t re-run and `modules` info can become stale. Please add `wizardMode` and `epelRepoUrlByDistribution` to the dependency array, and consider including any dependencies of `distroUrls` as well so the effect accurately tracks all inputs.
</issue_to_address>
### Comment 2
<location path="src/Components/CreateImageWizard/steps/Packages/components/RemovePackageButton.tsx" line_range="23" />
<code_context>
+ <Button
+ variant='plain'
+ icon={<MinusCircleIcon />}
+ aria-label='Remove package'
+ onClick={() => onRemove(item)}
+ isInline
</code_context>
<issue_to_address>
**nitpick (bug_risk):** The aria-label is package-specific but this button is also used for groups, which can confuse assistive technologies.
Given this component is used for both `IBPackageWithRepositoryInfo` and `GroupWithRepositoryInfo`, a fixed `aria-label='Remove package'` is inaccurate when the target is a group. Consider either using a generic label (e.g. `Remove item`) or accepting an `ariaLabel` prop so callers can specify `Remove package` or `Remove group` as needed.
</issue_to_address>
### Comment 3
<location path="src/Components/CreateImageWizard/steps/Packages/tests/Packages.test.tsx" line_range="122-131" />
<code_context>
- test('displays search bar and toggle buttons', async () => {
+ test('displays package type dropdown and search bar', async () => {
renderPackagesStep();
expect(
- await screen.findByRole('textbox', { name: /search packages/i }),
- ).toBeInTheDocument();
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for the "Search repositories outside of this image" flow in the new search UI
The new `PackageSearch` empty state includes a `ManageRepositoriesButton` and a secondary action to "Search repositories outside of this image" that toggles `isSearchingOtherRepos` and re-runs the search against EPEL. Current tests only check the "No results for \"asdf\" in selected repositories" text and don’t cover this flow. Please add tests that:
- start from a cloud search with no results in selected repos,
- click "Search repositories outside of this image" and assert that `PackageSearch` enters the "other repos" mode (e.g. EPEL banner or recommended results),
- optionally cover the case where there are still no results, asserting the final fallback message.
This will better protect the cross-repo search behavior over time.
Suggested implementation:
```typescript
describe('Search Functionality', () => {
test('displays package type dropdown and search bar', async () => {
renderPackagesStep();
expect(
await screen.findByRole('button', { name: /individual packages/i }),
).toBeInTheDocument();
expect(
await screen.findByRole('textbox', { name: /search packages/i }),
).toBeInTheDocument();
});
test('allows searching repositories outside of this image when cloud search has no results', async () => {
renderPackagesStep();
const user = userEvent.setup();
// Start from a cloud search that yields no results in selected repos
const searchInput = await screen.findByRole('textbox', {
name: /search packages/i,
});
await user.clear(searchInput);
await user.type(searchInput, 'asdf');
// Depending on implementation, the search may trigger on change or on submit.
// If a submit is required, uncomment the following line and ensure there is a submit button:
// await user.click(await screen.findByRole('button', { name: /search/i }));
// Initial empty-state message for selected repositories
expect(
await screen.findByText(
/no results for "asdf" in selected repositories/i,
),
).toBeInTheDocument();
// Click "Search repositories outside of this image"
const searchOutsideButton = await screen.findByRole('button', {
name: /search repositories outside of this image/i,
});
await user.click(searchOutsideButton);
// PackageSearch should now be in "other repos" mode.
// Assert on something that only appears in this mode:
// - an EPEL banner, or
// - a "Search only within this image" toggle to return to the original mode.
const otherReposToggle = await screen.findByRole('button', {
name: /search only within this image/i,
});
expect(otherReposToggle).toBeInTheDocument();
// Optionally, if an EPEL banner or recommended results are shown, assert on them as well.
// Adjust the text matchers here to the actual implementation (e.g. "Provided by EPEL").
// Example:
// expect(
// await screen.findByText(/epel/i),
// ).toBeInTheDocument();
});
test('shows final fallback when there are no results even in other repositories', async () => {
renderPackagesStep();
const user = userEvent.setup();
const searchInput = await screen.findByRole('textbox', {
name: /search packages/i,
});
await user.clear(searchInput);
await user.type(searchInput, 'asdf');
// Wait for initial "no results in selected repositories" state
expect(
await screen.findByText(
/no results for "asdf" in selected repositories/i,
),
).toBeInTheDocument();
// Trigger cross-repo search
const searchOutsideButton = await screen.findByRole('button', {
name: /search repositories outside of this image/i,
});
await user.click(searchOutsideButton);
// When the cross-repo search also yields no results, we should show a final fallback.
// Adjust the text here to match the actual UX copy for the "no results in any repositories" state.
expect(
await screen.findByText(/no results for "asdf" in any repositories/i),
).toBeInTheDocument();
});
```
1. Ensure `userEvent` from `@testing-library/user-event` is imported at the top of the file if it is not already:
`import userEvent from '@testing-library/user-event';`
2. The exact text matchers in the new tests (e.g. `"No results for \"asdf\" in selected repositories"`, `"Search repositories outside of this image"`, `"Search only within this image"`, and `"No results for \"asdf\" in any repositories"`) must be aligned with the actual strings used in `PackageSearch` / `ManageRepositoriesButton` and the empty-state components.
3. If the search is not triggered on input change but via a submit/enter action, wire the test to submit the search appropriately (e.g. clicking a "Search" button or sending `{enter}` to the text field).
4. If the "other repos" mode is indicated by specific UI elements (e.g. an EPEL banner or recommended package cards) rather than a "Search only within this image" button, update the expectations in the first new test to assert on those actual elements.
5. If your mock server (MSW or similar) currently returns non-empty results for the cross-repo/EPEL search, add or adjust handlers to produce the "no results in any repositories" scenario so the second test reliably exercises the final fallback message.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/Components/CreateImageWizard/steps/Packages/components/PackageSearch.tsx
Show resolved
Hide resolved
src/Components/CreateImageWizard/steps/Packages/components/RemovePackageButton.tsx
Outdated
Show resolved
Hide resolved
Sure, further clean up is possible, but maybe in a separate PR.
Same here, the "duplicated" logic is just a condition and a dispatch. |
This moves queries and states based on the component that uses them. This significantly lowers the amount of prop drilling.
3a28ff9 to
64c1c22
Compare
This updates the base behaviour and tests.
The step is not yet 1:1 with the mocks and some cleanup will be still needed, but the PR is already quite big.